Conversation
felixhekhorn
left a comment
There was a problem hiding this comment.
Remember to run pre-commit
|
fix the required changes, Apologies for not remembering pre-commit.
and also changed the name from load_meta_and_cards_from_tar → read_eko_cards and for test function name too. |
felixhekhorn
left a comment
There was a problem hiding this comment.
- Please mark conversation which you believe to have addressed as resolved, so we know more easily which problems still need to be addressed
- A simple way for me (and everybody else 🙃 ) to see you are human and not a bot is by seeing you thinking. For example e2b6366 could have been 4 commits - doing in turn what you describe here. Making small self-contained commits is not easy and I openly admit I'm not doing it myself all the time, but I try - and this way you can see me thinking.
- this will also help with writing more informative commit messages as "fix the required changes" is not very helpful in the long run
|
wait you're still thinking i am a bot 🥲. |
nono, of course not - but I want to help you to avoid that impression PS: and to teach you how to write better commits along the way 🙈 |
07e0266 to
9196f58
Compare
|
Sorry for the mess! I forgot to run pre-commit before pushing those changes. |
|
Also added the type hints for the newly added functions , |
felixhekhorn
left a comment
There was a problem hiding this comment.
last missing item: please add an entry to the Changelog
0879755 to
849b886
Compare
|
You probably saw it 😅 here's what happened, so you know the story: End result is the six separate commits above, one per change, plus the changelog entry. Learned a fair bit about rebase/upstream in the process 🙂. Happy to adjust anything,but I'd lean toward keeping them as separate commits since that matches what you asked for earlier. Let me know. |
felixhekhorn
left a comment
There was a problem hiding this comment.
Sorry another minor thing 🙃
aaand I'm learning something myself by reading PEP673 more carefully and understanding that Self is not always correct, but sometimes you need the literal name (however, not here, if I understood correctly 🙈 )
| return content | ||
|
|
||
| @classmethod | ||
| def from_raw(cls, raw: dict) -> "Metadata": |
There was a problem hiding this comment.
| def from_raw(cls, raw: dict) -> "Metadata": | |
| def from_raw(cls, raw: dict) -> Self: |
we can use Self here since a) we are already requiring py3.11 and b) we return an instance of cls and not Metadata explicitly (see also PEP 673).
change here and also runcards.py
Indeed, I prefer separate commits since this way you can see the problem evolve and you can discard or retain individual steps if necessary |
|
In case this applies to you: note e5d0050 |
Sure, I used Claude , via its chat interface, throughout this PR to understand the code and the issue, discuss the design, draft and refine code, and help debug. I reviewed, ran, and tested all changes myself and take full responsibility for the final contribution 😊. |
|
By the way since its coming to close, I’d love to keep the momentum going! so can you suggest what i should do next ? 🫠 |
closes #533
Implements the cheap card-reading we discussed in #533.
Approach (following your instructions )
from_rawtoTheoryCard,OperatorCard, andMetadata(in runcards.py / metadata.py). Each takes raw yaml, applies the v1/v2 upgrades, then callsfrom_dict. This is the upgrade logic lifted out of theEKO.theory_card/EKO.operator_cardproperties.load_meta_and_cards_from_tar(path)in runcards.py: opens the archive and pulls only metadata.yaml / theory.yaml / operator.yaml viatarfile.extractfile— the operators are never extracted. Order is metadata → theory → operator (operator upgrade needs the theory dict).from_raw, so folder-reading and tar-reading share one code path.Points I'd like your view on
v1/v2.update_metadataneeds the folder (paths), which the tar path doesn't have. So I kept that legacy 0.13/0.14 upgrade insideMetadata.loadand madefrom_rawdo the plain build. Consequence: reading a very old EKO straight from tar won't auto-upgrade its metadata. Fine for modern files — is that an acceptable boundary, or do you want it handled differently?load_meta_and_cards_from_tarin runcards.py as you suggested.Existing io tests pass locally and below is the new one.
